[tune] Better error message for Tune nested tasks / actors #25241

ericl · 2022-05-26T22:24:53Z

Why are these changes needed?

This PR uses a task/actor launch hook to generate better error messages for nested Tune tasks/actors in the case there are no extra resources reserved for them. The idea is that the Tune trial runner actor can set a hook prior to executing the user code. If the user code launches a task, and the placement group for the trial cannot possibly fit the task, then we raise TuneError right off to warn the user.

This isn't perfect, but should catch most common use cases and we can improve the docs as well to help cover this footgun.

Sample code:

import ray
from ray import tune

@ray.remote
def foo():
    pass
   
def objective(*args):
    ray.get(foo.remote())

tune.run(objective)

Sample output:

ray.tune.error.TuneError: No trial resources are available for executing the task `x.foo`. To
resolve this, specify the Tune option:

>  resources_per_trial=tune.PlacementGroupFactory(
>    [{'CPU': 1.0}] + [{'CPU': 1.0}] * N
>  )

Where `N` is the number of slots to reserve for trial tasks. For more information, refer to:
https://docs.ray.io/en/latest/tune/tutorials/tune-resources.html

Related issue number

Closes #25239

ericl · 2022-05-26T22:25:36Z

python/ray/remote_function.py

+        raise AbortTrialExecution(
+            f"Your trial launched the Ray task `{name}`, but no extra CPUs were "
+            "reserved for task execution. Specify the Tune argument "
+            '`per_trial_task_resources={"cpu": N}` to reserve CPUs for task execution.')


The user could also specify the placement group factory.

We should settle down on whether we want PGF to be end user API. It's pretty unintuitive IMO.

python/ray/remote_function.py

xwjiang2010 · 2022-06-01T04:17:06Z

doc/source/data/doc_code/key_concepts.py

-    ],
-)
-ray.get(placement_group.ready())
+# Tune launches its trainable functions in placement groups.


Hmmm, would this be a bit of a jump for users as why we are suddenly talking about using dataset in the context of Tune? Should we add a bit of context as to why this is an important key concept to understand? Like resource contention, hanging etc.

I updated the text around this section to clarify this.

xwjiang2010 · 2022-06-01T04:19:02Z

python/ray/tune/session.py

+    # Setup hooks for generating placement group resource deadlock warnings.
+    from ray import actor, remote_function
+
+    if "TUNE_DISABLE_RESOURCE_CHECKS" not in os.environ:


Do we want to add to doc/source/tune/api_docs/env.rst? Or we intend this to be more of an internal feature flag?

Would be great if you could add this to the docs.

Also

Suggested change

if "TUNE_DISABLE_RESOURCE_CHECKS" not in os.environ:

if os.environ.get("TUNE_DISABLE_RESOURCE_CHECKS", "0") != "1":

to follow convention with other tune env variables

I'd like to keep this a hidden / internal flag only, that's not documented. It should only be used for internal debugging.

xwjiang2010 · 2022-06-01T04:29:39Z

python/ray/tune/tests/test_warnings.py

+    )
+
+
+def test_scheduling_strategy_override():


Hmmm I got lost here. Can you add a few comments about what this is trying to capture?

xwjiang2010 · 2022-06-01T04:47:51Z

python/ray/tune/session.py

+    resources = {k: float(v) for k, v in resources.items() if v > 0}
+
+    raise TuneError(
+        f"No trial resources are available for executing the task `{name}`. "


launching the {submitted}: {name}?

python/ray/tune/session.py

krfricke

This is great, thanks, only minor nits/suggestions

python/ray/tune/session.py

krfricke · 2022-06-01T07:31:13Z

python/ray/tune/session.py

+        f">    [{main_resources}] + [{resources}] * N\n"
+        ">  )\n\n"
+        f"Where `N` is the number of slots to reserve for trial {submitted}s. "
+        "For more information, refer to: https://docs.ray.io/en/latest/tune/"


Suggestion for wording

Suggested change

"For more information, refer to: https://docs.ray.io/en/latest/tune/"

"If you are using a Ray training library, there might be a utility "

"function to set this automatically for you. "

"For more information, refer to: https://docs.ray.io/en/latest/tune/"

krfricke · 2022-06-01T07:37:57Z

python/ray/tune/session.py

+    # Setup hooks for generating placement group resource deadlock warnings.
+    from ray import actor, remote_function
+
+    if "TUNE_DISABLE_RESOURCE_CHECKS" not in os.environ:


Would be great if you could add this to the docs.

Also

Suggested change

if "TUNE_DISABLE_RESOURCE_CHECKS" not in os.environ:

if os.environ.get("TUNE_DISABLE_RESOURCE_CHECKS", "0") != "1":

to follow convention with other tune env variables

Co-authored-by: xwjiang2010 <[email protected]>

krfricke

Thanks!

ericl · 2022-06-03T18:57:49Z

python/ray/data/read_api.py

@@ -248,7 +248,7 @@ def read_datasource(
        )

    if len(read_tasks) < parallelism and (
-        len(read_tasks) < ray.available_resources().get("CPU", parallelism) // 2
+        len(read_tasks) < ray.available_resources().get("CPU", 1) // 2


If CPU == 0 / empty, fallback to 1 instead of parallelism (too large).

ericl commented May 26, 2022

View reviewed changes

python/ray/remote_function.py Outdated Show resolved Hide resolved

xwjiang2010 reviewed May 27, 2022

View reviewed changes

python/ray/remote_function.py Outdated Show resolved Hide resolved

ericl requested review from scv119, clarkzinzow, jjyao and maxpumperla as code owners May 28, 2022 04:47

ericl changed the title ~~[WIP / RFC] Better error message for Tune nested tasks / actors~~ [tune] Better error message for Tune nested tasks / actors May 28, 2022

ericl assigned richardliaw, xwjiang2010 and krfricke May 28, 2022

ericl added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label May 28, 2022

xwjiang2010 reviewed Jun 1, 2022

View reviewed changes

krfricke reviewed Jun 1, 2022

View reviewed changes

ericl requested a review from jianoaix as a code owner June 1, 2022 20:51

ericl removed the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jun 1, 2022

squash

66794d5

ericl force-pushed the tune-hook-poc branch from 0f99763 to 66794d5 Compare June 2, 2022 02:31

ericl added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jun 2, 2022

ericl and others added 2 commits June 3, 2022 11:43

Update python/ray/tune/session.py

7e6ca9e

Co-authored-by: xwjiang2010 <[email protected]>

Merge remote-tracking branch 'upstream/master' into tune-hook-poc

8fd3391

ericl removed the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Jun 3, 2022

krfricke approved these changes Jun 3, 2022

View reviewed changes

xwjiang2010 approved these changes Jun 3, 2022

View reviewed changes

ericl commented Jun 3, 2022

View reviewed changes

jianoaix approved these changes Jun 3, 2022

View reviewed changes

clarkzinzow approved these changes Jun 3, 2022

View reviewed changes

ericl merged commit 22aaf47 into ray-project:master Jun 3, 2022

justinvyu mentioned this pull request Aug 29, 2023

[train] New persistence mode: Finish migrating Tune tests + examples (small) #39047

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tune] Better error message for Tune nested tasks / actors #25241

[tune] Better error message for Tune nested tasks / actors #25241

ericl commented May 26, 2022 •

edited

Loading

ericl May 26, 2022

xwjiang2010 May 27, 2022

xwjiang2010 Jun 1, 2022

ericl Jun 1, 2022

xwjiang2010 Jun 1, 2022

krfricke Jun 1, 2022

ericl Jun 1, 2022

krfricke Jun 3, 2022

xwjiang2010 Jun 1, 2022

ericl Jun 1, 2022

xwjiang2010 Jun 1, 2022

ericl Jun 1, 2022

krfricke left a comment

krfricke Jun 1, 2022

ericl Jun 1, 2022

krfricke Jun 1, 2022

krfricke left a comment

ericl Jun 3, 2022 •

edited

Loading

	if "TUNE_DISABLE_RESOURCE_CHECKS" not in os.environ:
	if os.environ.get("TUNE_DISABLE_RESOURCE_CHECKS", "0") != "1":

[tune] Better error message for Tune nested tasks / actors #25241

[tune] Better error message for Tune nested tasks / actors #25241

Conversation

ericl commented May 26, 2022 • edited Loading

Why are these changes needed?

Related issue number

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krfricke left a comment

Choose a reason for hiding this comment

ericl Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

ericl commented May 26, 2022 •

edited

Loading

ericl Jun 3, 2022 •

edited

Loading